Feature/deseng692 and deseng671: Updated video widget, made crucial changes to authoring loaders and actions, added columns to engagement and widget_video tables. #2592

jareth-whitney · 2024-09-19T00:19:59Z

Issue #: https://citz-gdx.atlassian.net/browse/DESENG-692
Issue #: https://citz-gdx.atlassian.net/browse/DESENG-671

Description of changes:

Implemented video widget Figma design
Created custom layover bar for videos that shows video provider and has a custom logo
Transcripts will not be implemented at this time
Updated db to add the column "title" to "widget_video" table. As per design, user should be able to set custom title.
Updated db to add the column "description_title" to "engagement" table to accomodate the section header for the description
Also updated translation data structures as needed
Made fixes to the authoring section for better loading and actions

User Guide update ticket (if applicable):
N/A

…ital fixes to action and loading logic in authoring section, added columns to engagement and widget_video tables.

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

codecov-commenter · 2024-09-19T00:23:29Z

Codecov Report

Attention: Patch coverage is 32.43243% with 50 lines in your changes missing coverage. Please review.

Project coverage is 75.12%. Comparing base (5373540) to head (132ccb0).

Files with missing lines	Patch %	Lines
...agement/old-view/widgets/Video/VideoWidgetView.tsx	15.78%	48 Missing ⚠️
...src/met_api/services/widget_translation_service.py	0.00%	1 Missing ⚠️
...s/engagement/form/EngagementWidgets/Video/Form.tsx	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2592      +/-   ##
==========================================
- Coverage   75.27%   75.12%   -0.15%     
==========================================
  Files         612      612              
  Lines       22145    22215      +70     
  Branches     1839     1859      +20     
==========================================
+ Hits        16669    16689      +20     
- Misses       5203     5253      +50     
  Partials      273      273

Flag	Coverage Δ
metapi	`87.15% <90.90%> (+<0.01%)`	⬆️
metweb	`64.12% <22.22%> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
met-api/src/met_api/models/engagement.py	`78.97% <100.00%> (+0.10%)`	⬆️
...t-api/src/met_api/models/engagement_translation.py	`96.96% <100.00%> (+0.04%)`	⬆️
met-api/src/met_api/models/widget_translation.py	`98.03% <100.00%> (+0.03%)`	⬆️
met-api/src/met_api/models/widget_video.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/schemas/engagement.py	`93.44% <100.00%> (+0.10%)`	⬆️
...-api/src/met_api/schemas/engagement_translation.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/schemas/widget_translation.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/schemas/widget_video.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/services/engagement_service.py	`71.07% <ø> (ø)`
...met_api/services/engagement_translation_service.py	`89.00% <100.00%> (+0.11%)`	⬆️
... and 7 more

…lot suggestion.

Baelx · 2024-09-19T16:46:58Z

met-web/src/components/engagement/form/EngagementWidgets/Video/Form.tsx

+                                <ControlledTextField
+                                    name="title"
+                                    variant="outlined"
+                                    label=" "


Is there some other labelling happening here for screen readers?

Thanks Alex, I'll create some labels for screen readers :)

NatSquared · 2024-09-19T17:48:40Z

met-web/src/components/engagement/admin/create/authoring/AuthoringContext.tsx

    banner_filename: string;
    status_block: string[];
    title: string;
    icon_name: string;
    metadata_value: string;
-    send_report: boolean;
+    send_report: boolean | undefined;


Suggested change

send_report: boolean | undefined;

send_report?: boolean;

I need a state for the boolean that is neither true or false, so that if that field isn't set, it isn't submitted in the action. I'm open to suggestions if there's a better way to do it.

The question mark means that it's optional; i.e. it can be undefined

NatSquared · 2024-09-19T18:03:40Z

met-web/src/components/engagement/admin/create/authoring/AuthoringContext.tsx

                banner_filename: data.banner_filename,
                status_block: data.status_block,
                title: data.title,
                icon_name: data.icon_name,
                metadata_value: data.metadata_value,
-                send_report: getSendReportValue(data.send_report),
+                send_report: data.send_report ? getSendReportValue(data.send_report) : '',


Remove the getSendReportValue function - this code replicates the same behaviour. If you want it to store the string "false" instead of "" when send_report is turned off, replace the || with a ?? nullish coalescing operator.

Suggested change

send_report: data.send_report ? getSendReportValue(data.send_report) : '',

send_report: (data.send_report || '').toString()

I see the complete solution now. I'll change the logic.

NatSquared · 2024-09-19T18:04:37Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

+    // Update the loader data when the authoring section is changed, by triggering navigation().
+    const navigate = useNavigate();
+    useEffect(() => {
+        engagementData.then(() => navigate('.', { replace: true }));


Strongly consider using revalidator.revalidate() for this

NatSquared · 2024-09-19T18:05:16Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

@@ -42,25 +47,48 @@ const AuthoringSummary = () => {
        }
    }, [fetcher.data]);

-    const { content } = useRouteLoaderData('authoring-loader') as {
-        content: Promise<EngagementContent[]>;
+    const untouchedDefaultValues: EngagementUpdateData = {


Is there such a thing as "touched default values"? What does "default" mean here?

The defaultValues are the defaultValues for the form. They are passed down from the AuthoringContext and so is setDefaultValues, so that we have full control over the value. It represents the "untouched" state of the form, which will change based on the values that we load from the db. So the regular defaultValues are whatever is loaded into the form. The untouched default values are the blank default values that are never changed.

NatSquared · 2024-09-19T18:12:40Z

CHANGELOG.MD

 ## September 12, 2024
+


Make sure to also document the new work you did for the Summary section :)

NatSquared · 2024-09-19T18:48:33Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+    const formatVideoDuration = (duration: number) => {
+        let minutes = 0;
+        let hours = 0;
+        let seconds = 0;
+        if (3600 < duration) {
+            hours = Math.floor(duration / 3600);
+            minutes = Math.floor((duration - hours * 3600) / 60);
+            seconds = Math.floor(duration - hours * 3600 - minutes * 60);
+            return `${hours} hr ${minutes} min ${seconds} sec`;
+        } else if (3600 > duration && 60 < duration) {
+            minutes = Math.floor(duration / 60);
+            seconds = Math.floor(duration - minutes * 60);
+            return `${minutes} min ${seconds} sec`;
+        } else if (60 > duration && 0 < duration) {
+            return `${seconds} seconds`;
+        } else {
+            return '';
+        }
+    };


Suggested change

const formatVideoDuration = (duration: number) => {

let minutes = 0;

let hours = 0;

let seconds = 0;

if (3600 < duration) {

hours = Math.floor(duration / 3600);

minutes = Math.floor((duration - hours * 3600) / 60);

seconds = Math.floor(duration - hours * 3600 - minutes * 60);

return `${hours} hr ${minutes} min ${seconds} sec`;

} else if (3600 > duration && 60 < duration) {

minutes = Math.floor(duration / 60);

seconds = Math.floor(duration - minutes * 60);

return `${minutes} min ${seconds} sec`;

} else if (60 > duration && 0 < duration) {

return `${seconds} seconds`;

} else {

return '';

}

};

const formatVideoDuration = (duration: number) => {

if (duration <= 0) return '';

const hours = Math.floor(duration / 3600);

const minutes = Math.floor((duration % 3600) / 60);

const seconds = Math.floor(duration % 60);

let result = '';

if (hours > 0) result += `${hours} hr `;

if (minutes > 0) result += `${minutes} min `;

if (seconds > 0 || result === '') result += `${seconds} sec`;

return result.trim();

};

There are a couple of reasons why I didn't implement it this way:

IMO you would still want all subsequent fields from the biggest unit that has a value, even if the subsequent value is 0

Although my own code is maybe 4 lines longer and is more basic, I find it more readable

NatSquared · 2024-09-19T18:53:09Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+    const getVideoSource = (url: string) => {
+        const hostname = new URL(url).hostname;
+        if ('youtube.com' === hostname || 'youtu.be' === hostname) {
+            return 'YouTube';
+        } else if ('vimeo.com' === hostname) {
+            return 'Vimeo';
+        } else if ('facebook.com' === hostname) {
+            return 'Facebook';
+        } else if ('twitch.tv' === hostname) {
+            return 'Twitch';
+        } else if ('soundcloud.com' === hostname) {
+            return 'SoundCloud';
+        } else if ('mixcloud.com' === hostname) {
+            return 'Mixcloud';
+        } else if ('dailymotion.com' === hostname) {
+            return 'DailyMotion';
+        } else {
+            return 'Unknown';
+        }
+    };


Replace this large if/else/else... statement with something like an object map.

const domains = { 'youtube.com': { site:"Youtube", icon: faYoutube }, 'youtu.be': { site:"Youtube", icon: faYoutube }, 'vimeo.com': { site: "Vimeo", icon: faVimeo }, ... } ... const videoSource = (hostname in domains) ? domains[hostname].site : "Unknown"

I ended up using an array of objects but I did something similar to make it much less verbose.

NatSquared · 2024-09-19T18:58:46Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+                <Grid item xs={12} sx={{ maxHeight: '0px', padding: '0 !important', margin: '0' }}>
+                    <VideoOverlay
+                        videoOverlayTitle={videoOverlayTitle}
+                        source={getVideoSource(videoWidget.video_url)}


calculate the video source once and store it as a constant instead of querying it repeatedly inside your JSX. Same goes for the title.

NatSquared · 2024-09-19T19:00:07Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+    const getIcon = (source: string) => {
+        if ('YouTube' === source) {
+            return faYoutube;
+        } else if ('Vimeo' === source) {
+            return faVimeo;
+        } else if ('Facebook' === source) {
+            return faFacebookF;
+        } else if ('Twitch' === source) {
+            return faTwitch;
+        } else if ('SoundCloud' === source) {
+            return faSoundcloud;
+        } else if ('Mixcloud' === source) {
+            return faMixcloud;
+        } else if ('DailyMotion' === source) {
+            return faDailymotion;
+        } else {
+            return faQuestionCircle;
+        }
+    };


Again, please replace this large conditional with an object map or other solution. I recommend using the same map to get the source name and icon because they are related 1-1

Used the same object array from before to solve this issue.

NatSquared · 2024-09-19T19:05:47Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+            return `${player.getInternalPlayer().element.title} (${source} video)`;
+        } else if ('Facebook' === source || 'Twitch' === source || 'DailyMotion' === source) {
+            return `${widgetTitle} (${source} video)`; // API doesn't return video title, use widget title.
+        } else if ('SoundCloud' === source || 'Mixcloud' === source) {


It seems like sound platforms are adding a lot of complexity to your title parsing. Were these added platforms requested by the PO?

None of the platforms were added, they were just always enabled. All I'm doing is handling all of the possible inputs. Of course, we can change this by URL checking with YUP on the form if it comes to that.

NatSquared · 2024-09-19T19:10:08Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

+                    icon={getIcon(source)}
+                    style={{ fontSize: '1.9rem', paddingRight: '0.65rem', color: '#FCBA19' }}
+                />{' '}
+                <span style={{ fontSize: '0.95rem' }}>{videoOverlayTitle}</span>


Please stop creating text with size 0.95rem or 0.9rem. This does not correspond to any value used in the designs, and causes rendering issues because it comes out to a fractional pixel value.
To match the designs in Figma, use one of the following:

<BodyText size="small">

fontSize: '0.875rem'

fontSize: '14px'

Must have missed the exact value, will update. Doesn't really seem to be an issue since browsers have been parsing fractional pixel values for a long time. Please be patient, it's important to be gracious when teaching each other things.

NatSquared

Thanks for your work on these tickets Jareth! A few things for you to take a look at 👀

…s, fixed issues with deferred data aborted errors.

NatSquared · 2024-09-20T18:28:05Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

@@ -39,7 +39,7 @@ export const getLanguageValue = (currentLanguage: string, languages: Language[])
 const AuthoringTemplate = () => {
    const { onSubmit, defaultValues, setDefaultValues, fetcher }: AuthoringContextType = useOutletContext();
    const { engagementId } = useParams() as { engagementId: string }; // We need the engagement ID quickly, so let's grab it from useParams
-    const { engagement } = useRouteLoaderData('single-engagement') as { engagement: Engagement };
+    const { engagement } = useLoaderData() as { engagement: Promise<Engagement> };


Suggested change

const { engagement } = useLoaderData() as { engagement: Promise<Engagement> };

const { engagement } = useLoaderData() as EngagementLoaderData;

NatSquared · 2024-09-20T18:28:42Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

-                    )}
-                </Await>
-            </Suspense>
+            <div style={{ marginTop: '2rem' }}>


Thanks for resolving this layout shift issue :)

NatSquared

Thanks for looking at my suggestions Jareth! I would still recommend refactoring your timestamp calculation to be more readable and I would appreciate you addressing the description_title controller but I won't block you from merging :)

NatSquared · 2024-09-20T18:33:12Z

met-web/src/components/engagement/old-view/widgets/Video/VideoWidgetView.tsx

-            return 'Unknown';
-        }
-    };
+    const videoSources: WidgetVideoSource[] = [


Really appreciate this cleaned up approach :)

…c changes as per Nat.

sonarcloud · 2024-09-20T19:41:08Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jareth-whitney added 3 commits September 18, 2024 16:49

feature/deseng692 and feature/deseng671: Updated video widget, made v…

90149b2

…ital fixes to action and loading logic in authoring section, added columns to engagement and widget_video tables.

feature/deseng692 and deseng691: Added API files and postman collection.

8121133

feature/deseng692 and deseng671: Updated changelog.

a434866

jareth-whitney requested review from Baelx and NatSquared September 19, 2024 00:19

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

feature/deseng692: Updateded strictness of URL checks as per the Copi…

edc1833

…lot suggestion.

Baelx reviewed Sep 19, 2024

View reviewed changes

NatSquared reviewed Sep 19, 2024

View reviewed changes

NatSquared requested changes Sep 19, 2024

View reviewed changes

feature/deseng692: Made revisions to PR as per Nat and Alex's comment…

da88a8a

…s, fixed issues with deferred data aborted errors.

jareth-whitney requested a review from NatSquared September 20, 2024 17:34

NatSquared reviewed Sep 20, 2024

View reviewed changes

NatSquared approved these changes Sep 20, 2024

View reviewed changes

feature/deseng692: Simplified description title controller, small mis…

132ccb0

…c changes as per Nat.

jareth-whitney merged commit f51812c into main Sep 20, 2024
14 of 15 checks passed

jareth-whitney deleted the feature/deseng692 branch September 20, 2024 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/deseng692 and deseng671: Updated video widget, made crucial changes to authoring loaders and actions, added columns to engagement and widget_video tables. #2592

Feature/deseng692 and deseng671: Updated video widget, made crucial changes to authoring loaders and actions, added columns to engagement and widget_video tables. #2592

jareth-whitney commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 •

edited

Loading

Baelx Sep 19, 2024

jareth-whitney Sep 19, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 19, 2024

NatSquared Sep 20, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 19, 2024

NatSquared Sep 19, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 19, 2024

NatSquared Sep 19, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 20, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 20, 2024

NatSquared Sep 19, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 20, 2024

NatSquared Sep 19, 2024

jareth-whitney Sep 20, 2024

NatSquared Sep 19, 2024 •

edited

Loading

jareth-whitney Sep 20, 2024

NatSquared left a comment

NatSquared Sep 20, 2024

NatSquared Sep 20, 2024 •

edited

Loading

NatSquared left a comment

NatSquared Sep 20, 2024

sonarcloud bot commented Sep 20, 2024

	send_report: data.send_report ? getSendReportValue(data.send_report) : '',
	send_report: (data.send_report \|\| '').toString()

	const { engagement } = useLoaderData() as { engagement: Promise<Engagement> };
	const { engagement } = useLoaderData() as EngagementLoaderData;

Feature/deseng692 and deseng671: Updated video widget, made crucial changes to authoring loaders and actions, added columns to engagement and widget_video tables. #2592

Feature/deseng692 and deseng671: Updated video widget, made crucial changes to authoring loaders and actions, added columns to engagement and widget_video tables. #2592

Conversation

jareth-whitney commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NatSquared Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NatSquared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NatSquared Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

NatSquared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 20, 2024

Quality Gate passed

codecov-commenter commented Sep 19, 2024 •

edited

Loading

NatSquared Sep 19, 2024 •

edited

Loading

NatSquared Sep 20, 2024 •

edited

Loading